Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/financial years #180

Merged
merged 26 commits into from
Jul 28, 2023
Merged

Feature/financial years #180

merged 26 commits into from
Jul 28, 2023

Conversation

BerendWouters
Copy link
Member

I'm still in doubt about the domain model of the FinancialYear.

I've added some unit tests nonetheless, so we can still change the implementation.

@BerendWouters BerendWouters requested a review from vyruz1986 June 24, 2023 15:31
@BerendWouters BerendWouters self-assigned this Jun 24, 2023
@BerendWouters BerendWouters linked an issue Jun 24, 2023 that may be closed by this pull request
Persistence/Repositories/AttachmentStorage.cs Outdated Show resolved Hide resolved
Persistence/Repositories/BankAccountRepository.cs Outdated Show resolved Hide resolved
Persistence/Repositories/FinancialYearRepository.cs Outdated Show resolved Hide resolved
Persistence/Repositories/MemberRepository.cs Outdated Show resolved Hide resolved
Persistence/HaSpManContext.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@vyruz1986 vyruz1986 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also missing an EFCore migration. I'm also wondering whether FInancialYear should become an AggregateRoot whjch owns many Transaction objects. The it is now, they would become 2 aggregate roots with relations between them, which is generally frowned upon in DDD.
A better would be to make FinanalYear the aggregate root, and make the current Transaction aggregate root, a value object of a financial year instead. In this case the TransactionRepository would need to be removed, and each interaction with a transaction would need to take please through the FinancialYear.
In this an EFCore configuration using OwnsMany(y => y.Transactions) would be needed from the FinancialYear config

This reverts commit 686d8d8.

# Conflicts:
#	Domain/Transaction.cs
@BerendWouters
Copy link
Member Author

BerendWouters commented Jun 24, 2023

I'm also missing an EFCore migration. I'm also wondering whether FInancialYear should become an AggregateRoot whjch owns many Transaction objects. The it is now, they would become 2 aggregate roots with relations between them, which is generally frowned upon in DDD. A better would be to make FinanalYear the aggregate root, and make the current Transaction aggregate root, a value object of a financial year instead. In this case the TransactionRepository would need to be removed, and each interaction with a transaction would need to take please through the FinancialYear. In this an EFCore configuration using OwnsMany(y => y.Transactions) would be needed from the FinancialYear config

I was opting for that last path too, but I did want to have this discussed. From a domain point of view, this approach makes to most sense. So: FinancialYear becomes the new aggregate root, Transaction is removed and becomes a value object of that newly created aggregate root.
I do wonder about the following though:
The existing transaction handlers are unaware of the Financial year concept. Am I correct in letting them exist, but instead of interacting with the Transaction (which it can't since we removed the ITransactionRepository), it should first figure out what FinancialYear the transaction belongs to, using the ReceivedDate of the transaction? This would imply that we could move transactions from one FinancialYear to another (as longs as they're not closed and so on).
The cool thing about using the FinancialYear as the aggregate root is that we only have to check if the transaction can be edited if the FinancialYear isn't closed, and not the transaction themselves. As a consequence, I don't think we'd need to add a Locked property on the Transaction.

The reason I didnt yet create a migration was to prevent too many up and down migrations during development.

@vyruz1986
Copy link
Collaborator

I'm also missing an EFCore migration. I'm also wondering whether FInancialYear should become an AggregateRoot whjch owns many Transaction objects. The it is now, they would become 2 aggregate roots with relations between them, which is generally frowned upon in DDD. A better would be to make FinanalYear the aggregate root, and make the current Transaction aggregate root, a value object of a financial year instead. In this case the TransactionRepository would need to be removed, and each interaction with a transaction would need to take please through the FinancialYear. In this an EFCore configuration using OwnsMany(y => y.Transactions) would be needed from the FinancialYear config

I was opting for that last path too, but I did want to have this discussed. From a domain point of view, this approach makes to most sense. So: FinancialYear becomes the new aggregate root, Transaction is removed and becomes a value object of that newly created aggregate root. I do wonder about the following though: The existing transaction handlers are unaware of the Financial year concept. Am I correct in letting them exist, but instead of interacting with the Transaction (which it can't since we removed the ITransactionRepository), it should first figure out what FinancialYear the transaction belongs to, using the ReceivedDate of the transaction? This would imply that we could move transactions from one FinancialYear to another (as longs as they're not closed and so on). The cool thing about using the FinancialYear as the aggregate root is that we only have to check if the transaction can be edited if the FinancialYear isn't closed, and not the transaction themselves. As a consequence, I don't think we'd need to add a Locked property on the Transaction.

The reason I didnt yet create a migration was to prevent too many up and down migrations during development.

Indeed that would impose a breaking change in that transactions could not be accessed by themselves, but would need to be accessed through their FY aggregate root.
Which means that would likely translate back to UI changes as well, as you would first need to select the financial year before being able to see/add/edit transactions...

@BerendWouters
Copy link
Member Author

I've updated the PR.

No migrations yet, but I think the domain change has been done.

However, some other issues I'm not sure if the current implementation is the best:

  • As you mentioned, handling transactions should be done through the aggregate root. I've removed the TransactionRepository, but allowed to find the FY via a transactionId. I'm not really sure if this goes against any practices. It feels off, but there's a difference between the UI and the actual domain model (which I think is allowed in DDD).
  • However, by introducing the FY as the aggregate root and DDD stating you should always start at the aggregate root, we will always track all transactions (and attachments) under one FY. This might introduce a performance penalty, but unsure. It's not quite clear to me to what DDD says about including a certain child item, but it feels like a bad practice.
  • Querying. DDD is aimed at managing the state of the object you're working on, but is if I remember correctly quite loose about getting data. For that reason, we still have a DbSet of Transactions, but only for querying purposes. I think this is fine, but we should always adhere the no-tracking configuration, when querying the DbContext. In the past I've used a query facade pattern to make this more strict, but that's just convention imho.

@vyruz1986
Copy link
Collaborator

  • Querying. DDD is aimed at managing the state of the object you're working on, but is if I remember correctly quite loose about getting data. For that reason, we still have a DbSet of Transactions, but only for querying purposes. I think this is fine, but we should always adhere the no-tracking configuration, when querying the DbContext. In the past I've used a query facade pattern to make this more strict, but that's just convention imho.

I think the DbSet will have to go anyway, because iirc an entity cannot have both it's own DbSet as well as be owned by another entity.
Offcourse you could expose an IQueryable from the DbContext which does something like FinancialYears.SelectMany(y => t>Transactions).AsNoTracking(), or go all the way as you mentioned and create an I<Entity>QueryStore next to I<Entity>Repository for query purposes

@BerendWouters
Copy link
Member Author

I knew this would happen: owned types don't support hierarchy (https://learn.microsoft.com/en-us/ef/core/modeling/owned-entities#current-shortcomings).

@BerendWouters BerendWouters marked this pull request as ready for review July 28, 2023 20:46
@vyruz1986 vyruz1986 merged commit 28f6ed2 into master Jul 28, 2023
@vyruz1986 vyruz1986 deleted the feature/financial-years branch July 28, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a board member i wanna be able to close our financial year
2 participants